feat(frontend): reject duplicate subnet, NSG, and MRG on cluster create - #6004
Jakob Gray (JakobGray) wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: JakobGray The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Skipping CI for Draft Pull Request. |
d8f9ac9 to
6e76b7c
Compare
There was a problem hiding this comment.
Pull request overview
This PR moves several platform “uniqueness” and NSG placement checks (previously enforced by Cluster Service) into the frontend’s admission/static validation so invalid cluster creates fail early with consistent error messages.
Changes:
- Prefetch subscription-scoped clusters and node pools during cluster CREATE admission and add best-effort checks rejecting duplicate managed RG, subnet, and NSG usage across clusters/node pools.
- Extend static validation so
networkSecurityGroupIdmust be in the same subscription and must not be in the cluster’s managed resource group. - Update unit/integration tests and test artifacts to reflect newly enforced CREATE-time validation behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test-integration/frontend/artifacts/FrontendCRUD/Cluster/experimental-features-no-afec/04-httpCreate-cluster-with-4.19/create-with-4.19.json | Updates test create payload resource IDs to avoid conflicts with new admission checks. |
| internal/validation/validate_cluster.go | Adds NSG “same subscription” and “not in managed RG” checks at the cluster-level validation layer. |
| internal/validation/validate_cluster_comprehensive_test.go | Adds/adjusts validation test coverage for new NSG subscription/RG constraints. |
| internal/validation/hcpopenshiftcluster_test.go | Updates test cluster setup to include an NSG ID consistent with the new rules. |
| internal/admission/admit_cluster.go | Introduces CREATE-only cross-cluster/nodepool uniqueness admission checks for MRG/subnet/NSG. |
| internal/admission/admit_cluster_test.go | Adds unit tests verifying the new CREATE-only admission checks. |
| frontend/pkg/frontend/cluster.go | Prefetches subscription clusters and their node pools to populate the admission context on CREATE. |
| nodePoolSubnet := nodePool.Properties.Platform.SubnetID | ||
| if strings.EqualFold(subnetID, nodePoolSubnet.String()) { |
| subnetPath := fldPath.Child("subnetId") | ||
| subnetID := newObj.SubnetID.String() | ||
| var errs field.ErrorList |
| nsgPath := fldPath.Child("networkSecurityGroupId") | ||
| nsgID := newObj.NetworkSecurityGroupID.String() | ||
| var errs field.ErrorList |
6e76b7c to
03e5c54
Compare
| nodePoolSubnet := nodePool.Properties.Platform.SubnetID | ||
| if strings.EqualFold(subnetID, nodePoolSubnet.String()) { |
| for _, existing := range admissionContext.SubscriptionClusters { | ||
| existingSubnet := existing.CustomerProperties.Platform.SubnetID | ||
| if strings.EqualFold(subnetID, existingSubnet.String()) { | ||
| errs = append(errs, field.Invalid( | ||
| subnetPath, | ||
| subnetID, | ||
| fmt.Sprintf("Subnet '%s' is already in use by another cluster", subnetID), | ||
| )) | ||
| break | ||
| } | ||
| } |
| for _, nodePool := range admissionContext.SubscriptionNodePools { | ||
| nodePoolSubnet := nodePool.Properties.Platform.SubnetID | ||
| if strings.EqualFold(subnetID, nodePoolSubnet.String()) { | ||
| errs = append(errs, field.Invalid( | ||
| subnetPath, | ||
| subnetID, | ||
| fmt.Sprintf("Subnet '%s' is already in use by another cluster", subnetID), | ||
| )) | ||
| break | ||
| } | ||
| } |
| for _, existing := range admissionContext.SubscriptionClusters { | ||
| existingNSG := existing.CustomerProperties.Platform.NetworkSecurityGroupID | ||
| if strings.EqualFold(nsgID, existingNSG.String()) { | ||
| errs = append(errs, field.Invalid( | ||
| nsgPath, | ||
| nsgID, | ||
| fmt.Sprintf("Network Security Group '%s' is already in use by another cluster", nsgID), | ||
| )) | ||
| break | ||
| } | ||
| } |
| // ClusterNodePools is the list of node pools belonging to the cluster, used | ||
| // for minor-version skew checks against the desired cluster version. | ||
| ClusterNodePools []ClusterAdmissionNodePool | ||
| // SubscriptionClusters lists cluster documents in the same subscription, used |
There was a problem hiding this comment.
Document whether it includes the cluster being processed itself or not
| // used to ensure a cluster subnet is not already assigned to another cluster's | ||
| // node pool on CREATE. | ||
| // The list is empty on UPDATE. | ||
| SubscriptionNodePools []*api.HCPOpenShiftClusterNodePool |
There was a problem hiding this comment.
Do we need to check this? don't we check that for nodepools the subnet must be part of the VNet of the cluster?
There was a problem hiding this comment.
This replicates the ValidateSubnetNotUsedByAnotherClusterNodePools() validation in CS. Node pool subnet must be in the same VNet as its parent cluster, but VNet sharing across clusters is not prevented
There was a problem hiding this comment.
Regarding subnets, does the following summarize the behavior?
in CS:
- We allow reusing VNets between clusters
- We do not allow reusing Subnets between clusters
- We enforce that the node pool subnets must belong to the parent cluster VNet
- We allow reusing Subnets between Node Pools within the same cluster
- We do not allow reusing Subnets between Node Pools across clusters
There was a problem hiding this comment.
That all sounds correct
| // prefetched before admission runs. Concurrent creates (or a create racing with a | ||
| // node pool create) using the same subnet can both succeed. | ||
| func admitClusterSubnetResourceID(_ context.Context, admissionContext *ClusterAdmissionContext, op operation.Operation, fldPath *field.Path, newObj *api.CustomerPlatformProfile) field.ErrorList { | ||
| if op.Type != operation.Create || newObj.SubnetID == nil { |
There was a problem hiding this comment.
can newObj.SubnetID be nil at this point? if not, remove the check
There was a problem hiding this comment.
In a regular end-to-end flow it can be expected that the field is set and caught further upstream. In unit or integration tests it could be that a cluster is defined that is not fully formed because it is isolating focus on something else. If the subnet isn't set it is safe to say there is no uniqueness concern and the validation can return early.
There was a problem hiding this comment.
We shouldn't condition the logic to unit tests/integration tests
There was a problem hiding this comment.
It seems we run admission even when we have validation failures. Because of this, this needs a check to avoid panicking. To be safe we can return an error if the assumption of the subnet being set is broken.
| // Best-effort only: compares against SubscriptionClusters prefetched before | ||
| // admission runs. Concurrent creates with the same MRG name can both succeed. | ||
| func admitClusterManagedResourceGroupName(_ context.Context, admissionContext *ClusterAdmissionContext, op operation.Operation, fldPath *field.Path, newObj *api.CustomerPlatformProfile) field.ErrorList { | ||
| if op.Type != operation.Create || len(newObj.ManagedResourceGroup) == 0 { |
There was a problem hiding this comment.
Can managedresourcegroup be nil at this point? if not, remove the check
There was a problem hiding this comment.
We shouldn't condition the logic to unit tests/integration tests
| // Best-effort only: compares against SubscriptionClusters prefetched before | ||
| // admission runs. Concurrent creates with the same NSG can both succeed. | ||
| func admitClusterNetworkSecurityGroupResourceID(_ context.Context, admissionContext *ClusterAdmissionContext, op operation.Operation, fldPath *field.Path, newObj *api.CustomerPlatformProfile) field.ErrorList { | ||
| if op.Type != operation.Create || newObj.NetworkSecurityGroupID == nil { |
There was a problem hiding this comment.
can newObj.SubnetID be nil at this point? if not, remove the check
There was a problem hiding this comment.
We shouldn't condition the logic to unit tests/integration tests
56d4b1c to
c8a96a1
Compare
| errs = append(errs, admitClusterManagedResourceGroupName(ctx, admissionContext, op, fldPath, &newObj)...) | ||
| errs = append(errs, admitClusterSubnetResourceID(ctx, admissionContext, op, fldPath, &newObj)...) | ||
| errs = append(errs, admitClusterNetworkSecurityGroupResourceID(ctx, admissionContext, op, fldPath, &newObj)...) |
| for _, existing := range admissionContext.SubscriptionClusters { | ||
| existingSubnet := existing.CustomerProperties.Platform.SubnetID | ||
| if strings.EqualFold(subnetID, existingSubnet.String()) { | ||
| errs = append(errs, field.Invalid( | ||
| subnetPath, | ||
| subnetID, | ||
| fmt.Sprintf("Subnet '%s' is already in use by another cluster", subnetID), | ||
| )) | ||
| break | ||
| } | ||
| } |
| for _, nodePool := range admissionContext.SubscriptionNodePools { | ||
| nodePoolSubnet := nodePool.Properties.Platform.SubnetID | ||
| if strings.EqualFold(subnetID, nodePoolSubnet.String()) { | ||
| errs = append(errs, field.Invalid( | ||
| subnetPath, | ||
| subnetID, | ||
| fmt.Sprintf("Subnet '%s' is already in use by another cluster", subnetID), | ||
| )) | ||
| break | ||
| } | ||
| } |
| for _, existing := range admissionContext.SubscriptionClusters { | ||
| existingNSG := existing.CustomerProperties.Platform.NetworkSecurityGroupID | ||
| if strings.EqualFold(nsgID, existingNSG.String()) { | ||
| errs = append(errs, field.Invalid( | ||
| nsgPath, | ||
| nsgID, | ||
| fmt.Sprintf("Network Security Group '%s' is already in use by another cluster", nsgID), | ||
| )) | ||
| break | ||
| } | ||
| } |
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
aac6f9e to
28127fb
Compare
| return field.ErrorList{field.Invalid( | ||
| fldPath, | ||
| value.String(), | ||
| fmt.Sprintf("must belong to the same VNet as subnetId '%s'", otherSubnet.Parent.String()), |
| mrgPath := fldPath.Child("managedResourceGroup") | ||
| if len(newObj.ManagedResourceGroup) == 0 { | ||
| return field.ErrorList{field.InternalError(mrgPath, errors.New("managedResourceGroup must be set"))} | ||
| } |
| subnetPath := fldPath.Child("subnetId") | ||
| if newObj.SubnetID == nil { | ||
| return field.ErrorList{field.InternalError(subnetPath, errors.New("subnetId must be set"))} | ||
| } |
| existingSubnet := existing.CustomerProperties.Platform.SubnetID | ||
| if existingSubnet == nil { | ||
| errs = append(errs, field.InternalError(subnetPath, errors.New("existing cluster is missing subnetId"))) | ||
| continue | ||
| } |
| nodePoolSubnet := nodePool.Properties.Platform.SubnetID | ||
| if nodePoolSubnet == nil { | ||
| errs = append(errs, field.InternalError(subnetPath, errors.New("existing node pool is missing subnetId"))) | ||
| continue | ||
| } |
| nsgPath := fldPath.Child("networkSecurityGroupId") | ||
| if newObj.NetworkSecurityGroupID == nil { | ||
| return field.ErrorList{field.InternalError(nsgPath, errors.New("networkSecurityGroupId must be set"))} | ||
| } |
| existingNSG := existing.CustomerProperties.Platform.NetworkSecurityGroupID | ||
| if existingNSG == nil { | ||
| errs = append(errs, field.InternalError(nsgPath, errors.New("existing cluster is missing networkSecurityGroupId"))) | ||
| continue | ||
| } |
| if op.Type == operation.Create { | ||
| subscriptionID := originalCluster.ID.SubscriptionID | ||
| clusterIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, "").List(ctx, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot list clusters for cluster admission: %w", err) | ||
| } | ||
| for _, cluster := range clusterIterator.Items(ctx) { | ||
| admissionContext.SubscriptionClusters = append(admissionContext.SubscriptionClusters, cluster) | ||
|
|
||
| nodePoolIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, cluster.ID.ResourceGroupName).NodePools(cluster.ID.Name).List(ctx, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot list node pools for cluster admission: %w", err) | ||
| } | ||
| for _, nodePool := range nodePoolIterator.Items(ctx) { | ||
| admissionContext.SubscriptionNodePools = append(admissionContext.SubscriptionNodePools, nodePool) | ||
| } | ||
| if err := nodePoolIterator.GetError(); err != nil { | ||
| return nil, fmt.Errorf("cannot list node pools for cluster admission: %w", err) | ||
| } | ||
| } |
| // EtcdDataEncryptionKeyManagementModeTypeCustomerManaged - Customer managed encryption key management mode type. | ||
| EtcdDataEncryptionKeyManagementModeTypeCustomerManaged EtcdDataEncryptionKeyManagementModeType = "CustomerManaged" | ||
| // EtcdDataEncryptionKeyManagementModeTypePlatformManaged - Platform managed encryption key management mode type. | ||
| // Not currently supported by Cluster Service; left defined so EnsureDefaults / Cosmos defaults keep |
There was a problem hiding this comment.
no need to mention "by Cluster Service". Our service in general doesn't support it.
| observed[key] = identityPath | ||
| } | ||
|
|
||
| for operatorName, identity := range newObj.ControlPlaneOperators { |
There was a problem hiding this comment.
Without sorting, are the tests deterministic?
Cluster Service no longer returns synchronous 400s for platform uniqueness checks after async CS create migration. Move those checks to the frontend so invalid cluster PUTs fail at admission time with the same error messages CS used in performSpecValidation. These checks are best-effort and may not catch concurrent creates. - Prefetch subscription clusters and node pools in newClusterAdmissionContext - Add admitClusterManagedResourceGroupName, admitClusterSubnetResourceID, and admitClusterNetworkSecurityGroupResourceID (CREATE only) - Add NSG same-subscription and not-in-MRG rules to static validation (parity with validateAroHcpClusterNetworkSecurityGroupResourceId) Co-authored-by: Cursor <cursoragent@cursor.com>
Mirror CS validateAroHcpSwiftSubnetSameVnet: vnetIntegrationSubnetId must use the same VNet as subnetId. Co-authored-by: Cursor <cursoragent@cursor.com>
Mirror CS validateAzureOperatorsAuthenticationManagedIdentitiesUniqueWithinCluster across service, control-plane, and data-plane operator identities.
28127fb to
f69cf81
Compare
| subnetPath := fldPath.Child("subnetId") | ||
| if newObj.SubnetID == nil { | ||
| return field.ErrorList{field.Required(subnetPath, "")} | ||
| } |
| nsgPath := fldPath.Child("networkSecurityGroupId") | ||
| if newObj.NetworkSecurityGroupID == nil { | ||
| return field.ErrorList{field.Required(nsgPath, "")} | ||
| } |
| if op.Type == operation.Create { | ||
| subscriptionID := originalCluster.ID.SubscriptionID | ||
| clusterIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, "").List(ctx, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot list clusters for cluster admission: %w", err) | ||
| } | ||
| for _, cluster := range clusterIterator.Items(ctx) { | ||
| admissionContext.SubscriptionClusters = append(admissionContext.SubscriptionClusters, cluster) | ||
|
|
||
| nodePoolIterator, err := f.resourcesDBClient.HCPClusters(subscriptionID, cluster.ID.ResourceGroupName).NodePools(cluster.ID.Name).List(ctx, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cannot list node pools for cluster admission: %w", err) | ||
| } | ||
| for _, nodePool := range nodePoolIterator.Items(ctx) { | ||
| admissionContext.SubscriptionNodePools = append(admissionContext.SubscriptionNodePools, nodePool) | ||
| } | ||
| if err := nodePoolIterator.GetError(); err != nil { | ||
| return nil, fmt.Errorf("cannot list node pools for cluster admission: %w", err) | ||
| } | ||
| } | ||
| if err := clusterIterator.GetError(); err != nil { |
| // EtcdDataEncryptionKeyManagementModeTypeCustomerManaged - Customer managed encryption key management mode type. | ||
| EtcdDataEncryptionKeyManagementModeTypeCustomerManaged EtcdDataEncryptionKeyManagementModeType = "CustomerManaged" | ||
| // EtcdDataEncryptionKeyManagementModeTypePlatformManaged - Platform managed encryption key management mode type. | ||
| // Not currently supported; left defined so EnsureDefaults / Cosmos defaults keep | ||
| // filling the historic value, but excluded from ValidEtcdDataEncryptionKeyManagementModeType until | ||
| // platform-managed etcd encryption is supported. | ||
| EtcdDataEncryptionKeyManagementModeTypePlatformManaged EtcdDataEncryptionKeyManagementModeType = "PlatformManaged" | ||
| ) | ||
|
|
||
| var ( | ||
| ValidEtcdDataEncryptionKeyManagementModeType = sets.New[EtcdDataEncryptionKeyManagementModeType]( | ||
| EtcdDataEncryptionKeyManagementModeTypeCustomerManaged, | ||
| EtcdDataEncryptionKeyManagementModeTypePlatformManaged, | ||
| // TODO: re-enable once platform-managed etcd encryption is supported. | ||
| // EtcdDataEncryptionKeyManagementModeTypePlatformManaged, | ||
| ) |
| "etcd": { | ||
| "dataEncryption": { | ||
| "keyManagementMode": "PlatformManaged" | ||
| "customerManaged": { | ||
| "encryptionType": "KMS", | ||
| "kms": { | ||
| "activeKey": { | ||
| "name": "vc-encryption-key", | ||
| "vaultName": "vc-key-vault", | ||
| "version": "2024-12-01-preview" | ||
| } |
| "etcd": { | ||
| "dataEncryption": { | ||
| "keyManagementMode": "PlatformManaged" | ||
| "customerManaged": { | ||
| "encryptionType": "KMS", | ||
| "kms": { | ||
| "activeKey": { | ||
| "name": "vc-encryption-key", | ||
| "vaultName": "vc-key-vault", | ||
| "version": "2024-12-01-preview" | ||
| } |
Align with CS validateAroHcpEtcdEncryptionDataEncryptionKeyManagementMode: only CustomerManaged is accepted until platform-managed etcd encryption is supported.
f69cf81 to
52a9c45
Compare
| subnetPath := fldPath.Child("subnetId") | ||
| if newObj.SubnetID == nil { | ||
| return field.ErrorList{field.Required(subnetPath, "")} | ||
| } |
| nsgPath := fldPath.Child("networkSecurityGroupId") | ||
| if newObj.NetworkSecurityGroupID == nil { | ||
| return field.ErrorList{field.Required(nsgPath, "")} | ||
| } |
| mrgPath := fldPath.Child("managedResourceGroup") | ||
| if len(newObj.ManagedResourceGroup) == 0 { | ||
| return field.ErrorList{field.Required(mrgPath, "")} | ||
| } |
|
/test integration |
|
/hold These changes will likely be incorporated as a part of #6082 |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Work merged in #6121 |
|
The same code has been merged in a unified PR with other changes, in #6121. Closing this. |
What
This change closes the following gaps in validation during cluster create between CS and ARO-HCP:
These mirror checks done in CS before a cluster create is accepted:
validateAroHcpClusterNetworkSecurityGroupResourceIdvalidateManagedResourceGroupUniquenessvalidateSubnetUniquenessvalidateNsgUniquenessThese checks are best-effort. Race conditions are possible for clusters admitted at the same time.
Why
Cluster Service no longer returns synchronous 400s for platform uniqueness checks after async CS create migration. Move those checks to the frontend so invalid cluster PUTs fail at admission time with the same error messages CS used in performSpecValidation.
Testing
Added unit tests for new functions. Modified integration tests where new errors were occurring because they are now being caught by the admission validation. Tested against existing E2E tests around subnet/NSG/MRG reuse between clusters (
test/e2e/cluster_nsg_subnet_reuse.goandtest/e2e/clusters_sharing_resgroup.go)Special notes for your reviewer
PR Checklist
If E2E tests are included:
demonstrate that the test is able to detect a defect/error and fail with
proper error message and logs which communicates nature of the problem.